fix: enable workflow validation before deployment (was hardcoded to false)#3579
fix: enable workflow validation before deployment (was hardcoded to false)#3579guoyangzhen wants to merge 5 commits intosimstudioai:mainfrom
Conversation
PR SummaryMedium Risk Overview The panel now computes The Written by Cursor Bugbot for commit 945b207. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR enables workflow validation before deployment by replacing a hardcoded Key findings:
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Panel (panel.tsx)
participant WorkflowStore
participant validateWorkflowState
participant DeployAPI (route.ts)
User->>Panel (panel.tsx): Edits workflow / opens panel
Panel (panel.tsx)->>WorkflowStore: useWorkflowStore selector
WorkflowStore-->>Panel (panel.tsx): { blocks, edges } (loops/parallels missing ⚠️)
Panel (panel.tsx)->>validateWorkflowState: validateWorkflowState({ blocks, edges })
validateWorkflowState-->>Panel (panel.tsx): { valid, errors }
Note over Panel (panel.tsx): hasValidationErrors controls Run button disabled state
User->>Panel (panel.tsx): Clicks Deploy
Panel (panel.tsx)->>DeployAPI (route.ts): POST /api/workflows/[id]/deploy
DeployAPI (route.ts)->>DeployAPI (route.ts): loadWorkflowFromNormalizedTables(id)
DeployAPI (route.ts)->>validateWorkflowState: validateWorkflowState({ blocks, edges }) (loops/parallels missing ⚠️)
validateWorkflowState-->>DeployAPI (route.ts): { valid, errors }
alt Validation fails (incl. false positives for loops/parallels)
DeployAPI (route.ts)-->>Panel (panel.tsx): 400 Workflow has validation errors
else Validation passes
DeployAPI (route.ts)->>DeployAPI (route.ts): validateWorkflowSchedules(...)
DeployAPI (route.ts)->>DeployAPI (route.ts): deployWorkflow(...)
DeployAPI (route.ts)-->>Panel (panel.tsx): 200 Deployed
end
Last reviewed commit: 70dd04d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const hasValidationErrors = useWorkflowStore((state) => { | ||
| if (Object.keys(state.blocks).length === 0) return false | ||
| const result = validateWorkflowState({ | ||
| blocks: state.blocks, | ||
| edges: state.edges, | ||
| }) | ||
| return !result.valid | ||
| }) |
There was a problem hiding this comment.
Missing loops and parallels causes false-positive validation errors
validateWorkflowState checks edges against loop and parallel container IDs (lines 270–292 of validation.ts). Because state.loops and state.parallels are not passed here, any workflow that uses a loop or parallel block will have all edges connected to those containers reported as referencing "non-existent" source/target blocks — causing hasValidationErrors to be true for valid workflows and permanently disabling the Run button.
| const hasValidationErrors = useWorkflowStore((state) => { | |
| if (Object.keys(state.blocks).length === 0) return false | |
| const result = validateWorkflowState({ | |
| blocks: state.blocks, | |
| edges: state.edges, | |
| }) | |
| return !result.valid | |
| }) | |
| const hasValidationErrors = useWorkflowStore((state) => { | |
| if (Object.keys(state.blocks).length === 0) return false | |
| const result = validateWorkflowState({ | |
| blocks: state.blocks, | |
| edges: state.edges, | |
| loops: state.loops, | |
| parallels: state.parallels, | |
| }) | |
| return !result.valid | |
| }) |
| const { validateWorkflowState } = await import('@/lib/workflows/sanitization/validation') | ||
| const stateValidation = validateWorkflowState({ | ||
| blocks: normalizedData.blocks, | ||
| edges: normalizedData.edges, | ||
| }) | ||
| if (!stateValidation.valid) { | ||
| logger.warn( | ||
| `[${requestId}] Workflow validation failed for ${id}: ${stateValidation.errors.join('; ')}` | ||
| ) | ||
| return createErrorResponse( | ||
| `Workflow has validation errors: ${stateValidation.errors.join('; ')}`, | ||
| 400 | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing loops and parallels blocks valid workflow deployments
normalizedData already contains loops and parallels (used in the GET handler on lines 83–86 of this same file), but they are omitted from the validateWorkflowState call here. The validation function uses workflowState.loops || {} and workflowState.parallels || {}, so passing undefined means every edge whose source or target is a loop/parallel container will be flagged as referencing a non-existent block — causing the deployment to be rejected with a 400 even for a perfectly valid workflow.
| const { validateWorkflowState } = await import('@/lib/workflows/sanitization/validation') | |
| const stateValidation = validateWorkflowState({ | |
| blocks: normalizedData.blocks, | |
| edges: normalizedData.edges, | |
| }) | |
| if (!stateValidation.valid) { | |
| logger.warn( | |
| `[${requestId}] Workflow validation failed for ${id}: ${stateValidation.errors.join('; ')}` | |
| ) | |
| return createErrorResponse( | |
| `Workflow has validation errors: ${stateValidation.errors.join('; ')}`, | |
| 400 | |
| ) | |
| } | |
| const stateValidation = validateWorkflowState({ | |
| blocks: normalizedData.blocks, | |
| edges: normalizedData.edges, | |
| loops: normalizedData.loops, | |
| parallels: normalizedData.parallels, | |
| }) |
| const hasValidationErrors = useWorkflowStore((state) => { | ||
| if (Object.keys(state.blocks).length === 0) return false | ||
| const result = validateWorkflowState({ | ||
| blocks: state.blocks, | ||
| edges: state.edges, | ||
| }) | ||
| return !result.valid | ||
| }) |
There was a problem hiding this comment.
Expensive validation runs on every Zustand store update
validateWorkflowState iterates all blocks, calls getBlock() and getTool() per block, and checks all edges — all synchronously inside a Zustand selector. Selectors run on every state change, including every single keystroke in any block's input field. For larger workflows this will cause perceptible UI lag.
Consider memoising outside the selector or debouncing:
// Coarse selector — only re-validates when blocks/edges identity changes
const { blocks, edges, loops, parallels } = useWorkflowStore(
useShallow((state) => ({
blocks: state.blocks,
edges: state.edges,
loops: state.loops,
parallels: state.parallels,
}))
)
const hasValidationErrors = useMemo(() => {
if (Object.keys(blocks).length === 0) return false
return !validateWorkflowState({ blocks, edges, loops, parallels }).valid
}, [blocks, edges, loops, parallels])This keeps the computation lazy but avoids re-running it on unrelated store updates (e.g. execution state, cursor position).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx
Outdated
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx
Outdated
Show resolved
Hide resolved
…recomputation - Use individual selectors for blocks/edges/loops/parallels with useShallow - Memoize validation result with useMemo, only recomputing when deps change - Pass shallow copies of state to validateWorkflowState to prevent any internal mutation from affecting Zustand store state Addresses Bugbot review feedback for simstudioai#3579
|
@cursor[bot] Thanks for the review! Fixed both issues:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| parallels: { ...(parallels || {}) }, | ||
| }) | ||
| return !result.valid | ||
| }, [blocks, edges, loops, parallels]) |
There was a problem hiding this comment.
Deploy button not disabled when validation errors exist
Medium Severity
The new hasValidationErrors value only feeds into isWorkflowBlocked → isButtonDisabled, which is wired to the Run button. The Deploy component doesn't receive hasValidationErrors as a prop and its own isDisabled (isDeploying || !canDeploy || isEmpty) doesn't account for validation errors. Users can still click Deploy on an invalid workflow. The backend will reject it, but the UI-layer prevention described in the PR is missing for the Deploy button.
| parallels: { ...(parallels || {}) }, | ||
| }) | ||
| return !result.valid | ||
| }, [blocks, edges, loops, parallels]) |
There was a problem hiding this comment.
Keyboard shortcut bypasses validation-disabled Run button
Medium Severity
The run-workflow keyboard shortcut handler (Mod+Enter) only checks isExecuting before calling runWorkflow(). It does not check hasValidationErrors or isButtonDisabled. Previously this didn't matter because hasValidationErrors was hardcoded to false. Now that it reflects real validation state, users can bypass the disabled Run button entirely by pressing the keyboard shortcut, running invalid workflows the UI is supposed to prevent.
Additional Locations (1)
1 participant
Problem
The
hasValidationErrorsflag inpanel.tsxwas hardcoded tofalsewith a TODO comment, allowing completely broken workflows to be deployed. The backend deploy route also only validated schedules, not workflow state.Users could deploy workflows with:
Fix
Frontend (
panel.tsx):const hasValidationErrors = falsewith actualvalidateWorkflowState()callBackend (
deploy/route.ts):validateWorkflowState()check before allowing deploymentDefense in Depth
Fix applied at both layers:
Fixes #3444